-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix StyxObservable type parameters #219
Fix StyxObservable type parameters #219
Conversation
return new StyxCoreObservable<>(delegate.map(transformation::apply)); | ||
} | ||
|
||
public <U> StyxObservable<U> flatMap(Function<T, StyxObservable<U>> transformation) { | ||
public <U> StyxObservable<U> flatMap(Function<? super T, ? extends StyxObservable<? extends U>> transformation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am skeptical about ? extends StyxObservable<? extends U>
bit. Due to some technical limitations, we don't intend StyxObservable
to be extendable (although it is implemented by a package private subclass).
But the above annotation suggests that it is OK to provide a mapping function that provides StyxObservable
subclasses. Perhaps the function signature needs to be just:
Function<? super T, StyxObservable<? extends U>> transformation
In fact this remark probably applies to all instances where this pattern is repeating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think StyxObservable is an interface, isn't it? :) In this case, both signatures would accept any implementation of 'StyxObservable' (mmm, provided the declared type was for StyxObservable?) , but I think the one used by Kyle/rxJava would allow to pass Function<StyxCoreObservable, SecondParam> whereas your version only allows Function<StyxObservable, SecondParam> .
I would go for your proposed change and simplify the code as, like you said, we are not going to have different implementations of StyxObservable and, thus, I don't see a reason to restrict the type of the (input type of the) function (StyxCoreObservable instead of StyxObservable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon making the suggested change to flatMap
, a lot of the scala Specs stop compiling. It's not completely clear to me why, but I think there may be some significance to the new signature style (as seen in Rx) beyond what we currently realise. I will continue investigating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have investigated a little bit an the problem with Function<? super T, StyxObservable<? extends U>>
is that it will only accept a function whose return type is verbatim StyxObservable<? extends U>
.
This means that a function that returns StyxObservable<B>
where B
is a subtype of U
will not be allowed.
For this reason we need the Function<? super T, ? extends StyxObservable<? extends U>>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, of course, <StyxObservable<Integer>>
is not compatible with <StyxObservable<? extends Number>>
due to the type invariance of the first/outer parametrized type (<StyxObservable>
). That´s what happens (to me) when reviewing code outside the IDE...
@@ -103,4 +83,21 @@ public StyxCoreObservable(CompletionStage<T> future) { | |||
return future; | |||
} | |||
|
|||
private static <U> Observable<? extends U> toObservable(StyxObservable<U> styxObservable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you see any advantage to the return type? It's usually considered a bad practice to include wildcard types in return types as, rather than providing more flexibility to the caller, it's just makes the code more bloated.
If we pass a StyxObservable U we are always going to return an Observable U and not any subclass of U.
BTW, T seems better than U if we want to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @dvlato I assume you are referring to these guidelines?
https://docs.oracle.com/javase/tutorial/java/generics/wildcardGuidelines.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know that was in the Java tutorials! I thought it was usually quoted by the 'Effective Java' book.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is easily fixed as it is private and doesn't break anything. As for which letters to use for type parameters, both Rx and java streams use T
for input and R
for output (where both are present), so it seems reasonable to use the same practice.
@@ -62,31 +50,23 @@ public StyxCoreObservable(CompletionStage<T> future) { | |||
return new StyxCoreObservable<T>(Observable.error(cause)); | |||
} | |||
|
|||
public <U> StyxObservable<U> map(Function<T, U> transformation) { | |||
public <U> StyxObservable<U> map(Function<? super T, ? extends U> transformation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather change the return type name to R (instead of U) following Java 8 (e.g, https://docs.oracle.com/javase/8/docs/api/java/util/function/Function.html) and Observable's convention.
Maybe just copy the generic names from the Observable interface for the same methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, we can add some small improvements.
The type parameters were overly strict.